Repository navigation
ddk: verify a signed contract from its three messages - #231
Merged
Merged
Conversation
Owner
Author
|
Note 🤖 Claude Opus 5.5 responding on behalf of bennyhodl Review against orange-grove's consumers ( This is the verifier orange-grove wants to share between the wallet's import and the server's submission check; it replaces about 200 lines of
|
This was referenced Oct 13, 2026
finalize_sign copied the offering party's funding witnesses out of the sign message without checking them, so a bad witness produced a funding transaction that could not confirm. Consumers that import or restore a contract had no public way to check the messages at all, and advanced::verify_cet_adaptor_signatures rebuilds under the current fee rule, so it rejects valid OwnPayoutOnly contracts. ddk::contract::verify_signed_contract rebuilds the contract under the fee rule its sign message names, then checks both parties' refund and CET adaptor signatures, every offer funding witness against its prevout, and every offer splice half against the previous contract's key. It returns a VerifiedContract with the contract id, the transactions, and the offer funding inputs whose script type cannot be checked (a P2WSH multisig, for example). Those are neither accepted nor rejected; finalize proceeds, and a consumer that needs every signature verified requires the list to be empty. A splice half may be followed by its offer key, nothing else. finalize_sign and finalize_sign_spliced run the verification before completing the funding transaction. Settlement shares the same refund and adaptor checks but verifies only the counterparty half it combines, so a refund never depends on the rest of the messages. ddk-dlc gains verify_funding_witness (P2WPKH, P2SH-P2WPKH, Taproot key path, SIGHASH_ALL only, script signature checked; other script types return the new Error::UnsupportedScriptType) and util::multisig_witness, which sign_multi_sig_input and combine_dlc_input_signatures now share. verify_dlc_funding_input_signature and combine_dlc_input_signatures reject a half that is not DER plus SIGHASH_ALL, and combine now returns a Result.
verify_signed_contract returned success for a contract whose offer funding witness spent a script type DDK cannot check, and listed the input in VerifiedContract::unverified_funding_inputs. Every caller then had to remember to look at that list before treating the contract as verified, and finalize_sign never did. A success now means everything the three messages carry was verified: an unverifiable offer witness fails with the new ContractError::UnverifiableFundingWitness, and unverified_funding_inputs is gone. No caller needed the lenient result: finalize_sign was its only user, and it is strict too. The accepting party's funding witnesses are not in the messages, so they stay outside what verify_signed_contract checks; the docs now say so up front. verify_funding_witness also checks the witness against the input's max_witness_len. The funding fee was paid on the declared length, so a longer witness pays less than the agreed rate. It fails with ddk_dlc::Error::WitnessTooLong, surfaced as ContractError::FundingWitnessTooLong. It also rejected too little for Taproot. bitcoin's Signature::from_slice reads a trailing 0x00 as the default sighash type, but BIP-341 makes a 65-byte signature with hash type 0x00 invalid; the default is only the 64-byte form. Such a witness passed verification and could not confirm.
bennyhodl
force-pushed
the
benny/225-verify-signed-contract
branch
from
October 15, 2026 18:11
4b72d44 to
785180c
Compare
RUSTSEC-2026-0385 (ctutils < 0.4.4) and RUSTSEC-2026-0414 (pulldown-cmark < 0.13.5) fail the audit job. Both are patch bumps in the lockfile only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Item 2 of #225.
apply_funding_signaturescopied the offering party's funding witnesses without checking them, andfinalize_signtrusted them.advanced::verify_cet_adaptor_signaturesrebuilds under the default fee rule, so a contract signed underOwnPayoutOnlycouldn't be verified through the public API. Lygos wrote its ownverify_signto cover all of this.API
ddk::contract::verify_signed_contract(offer, accept, sign) -> VerifiedContractpicks the fee rule from the sign message's contract id, the same waycreate_signed_dlc_transactionsdoes. It then checks:SIGHASH_ALLand the correctscript_sig;[half]or[half, key]; anything else is rejected.max_witness_len:ContractError::UnverifiableFundingWitness;max_witness_lenfails withContractError::FundingWitnessTooLong. The funding fee was paid on the declared length, so a longer witness pays less than the agreed rate.VerifiedContractexposescontract_id(),transactions()andinto_transactions().verify_funding_witness(secp, fund_tx, index, &TxInputInfo, prevouts, &Witness), plus newError::UnsupportedScriptTypeandError::WitnessTooLong.0x00is rejected. BIP-341 allows the default type only in the 64-byte form; bitcoin'sSignature::from_sliceaccepts it, and such a witness can't confirm.util::multisig_witness(script, (pk, sig), (pk, sig)).sign_multi_sig_inputandcombine_dlc_input_signaturesboth use it, so there is one 2-of-2 witness builder.Lifecycle
finalize_sign,finalize_sign_splicedand theadvanced::finalize_sign_*_with_funding_signaturesvariants runverify_signed_contractfirst, so they now refuse an offer funded from a script type DDK can't check, and an offer witness over its declared length. That replaces the separate offer-half check incomplete_dlc_input_witnesses, so there is one verification path. Finalize also picks the fee rule from the sign message now, instead of always using the current rule.context.rsare split intoverify_refund_signatureandverify_adaptor_signatures.sign.rs,advanced.rsandsettle.rsuse them instead of their own copies.advanced::verify_cet_adaptor_signaturesruns before a sign message exists, so it has no contract id to pick a fee rule. It is documented as such and points toverify_signed_contract.Breaking changes
finalize_signand its variants refuse a contract whose offer is funded from a P2WSH multisig (or any script other than P2WPKH, P2SH-P2WPKH or Taproot key path). An external signer that funded offers that way finalized before this PR and doesn't now.max_witness_lenis refused. A P2WPKH witness with a low-S signature is at most 108 bytes (1 + 1 + 72 + 1 + 33), so the standard declared 108 holds without low-R grinding.ContractErrorgainsUnverifiableFundingWitnessandFundingWitnessTooLong.combine_dlc_input_signaturestakes&[u8]and returnsResult<Witness, Error>. Add?at call sites.verify_dlc_funding_input_signaturerequires DER plusSIGHASH_ALL; it used to accept a compact signature. Callers holding a raw signature passutil::finalize_sig(&sig, EcdsaSighashType::All).ErrorgainsUnsupportedScriptTypeandWitnessTooLong.Not changed / follow-ups
signed_contextrather than consumingVerifiedContract. A refund should only need the counterparty's refund signature, and the keyless-settlement PR (ddk: settle a contract from its messages without a key #228) restructuressettle.rs. Settlement entry points onVerifiedContractshould follow once both land.remote_fund_pubkey. The stateless API checkslocal_fund_pubkey, as master already did.ffi/src/lib.rs:470) passes pre-sorted keys tocombine_dlc_input_signatureswhile the signatures stay in local/remote order. Whenlocal_pk > remote_pkthat builds an invalid witness. This bug predates this PR; it needs a fix in ddk-ffi, along with the new?.Testing
funding_witnesses_verify_for_each_supported_coin: all three coin types verify. Taproot SIGHASH_NONE, a P2WSH coin, and a missing, wrong or unexpectedscript_sigare each rejected.multisig_witness_orders_signatures_by_public_key.ddk/tests/stateless.rs:max_witness_len;finalize_signrejects a bad funding witness.contract::29, stateless 63, ddk-manager lib 62, all passing.stateless_execution --ignoredon regtest: single-funded, splice in/out, alternating splice chain, accept-party splice, numeric, disjoint and the DDK wallet signer all pass.cargo fmt --check,cargo clippy -- -D warnings,cargo check -p ddk --no-default-featuresandcargo check -p ddk-dlc --no-default-features --features no-stdpass.script_sigwas unchecked;The remaining points are listed above.
OwnPayoutOnly) and 2.0, passverify_signed_contract, and each rebuilds the funding txid on chain;verify_signreplaced byverify_signed_contract.